Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More implement arguments plain and from line #4950

Merged

Conversation

Ideflop
Copy link
Contributor

@Ideflop Ideflop commented Jun 5, 2023

Related to #2320

This implements the arguments plain and from-line.

The plain option does nothing because, according to the man page of more, it is written that this option is silently ignored as backwards compatibility.

This also fixes two bugs.

The first one occurs when multiple files are given, and a file takes one display to output its content. Now, it displays the next file message if there is another file and only stops if there is none other.
The second one happens with the squeeze option. When multiple files are given and the end of the file takes less than a full terminal size to display, it should not start printing the next file.

) -> UResult<()> {
let (cols, mut rows) = terminal::size().unwrap();
if let Some(number) = options.lines {
rows = number;
}

let lines = break_buff(buff, usize::from(cols));
let mut lines = break_buff(buff, usize::from(cols));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

according to the coverage, the test doesn't go here, would it be possible to test that? thanks

Copy link
Contributor Author

@Ideflop Ideflop Jun 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a test for it, but I believe the coverage still doesn't detect it because the tests for more are all contained within an if std::io::stdout().is_terminal() statement.

@Ideflop Ideflop requested a review from sylvestre June 20, 2023 09:01
@cakebaker
Copy link
Contributor

Hm, strange, my local more (from util-linux 2.39, on Archlinux) doesn't have a --from-line argument. It uses +<number> instead.

@Ideflop
Copy link
Contributor Author

Ideflop commented Jun 23, 2023

Hm, strange, my local more (from util-linux 2.39, on Archlinux) doesn't have a --from-line argument. It uses +<number> instead.

Yes, you are right.

The problem is that clap doesn't currently work with non-Unix options. There's an open issue about this problem at clap-rs/clap#2468

That's why I use what was given in #2320 (comment)

@tertsdiepraam
Copy link
Member

tertsdiepraam commented Jun 24, 2023

I think using "normal" arguments is fine, because more is not technically part of the coreutils, so we can be a bit more flexible about compatibility.

@cakebaker
Copy link
Contributor

I think there is a bug with paging up. When I run cargo run more -F100 README.md, the correct lines are shown, however, it is not possible to page up and go to the top of the file. Paging down works fine.

@Ideflop Ideflop force-pushed the more_implement_arguments_plain_and_from_line branch from b0d8192 to b6f53aa Compare June 26, 2023 19:51
@Ideflop Ideflop force-pushed the more_implement_arguments_plain_and_from_line branch from b6f53aa to c4c3a35 Compare June 26, 2023 19:59
@Ideflop
Copy link
Contributor Author

Ideflop commented Jun 26, 2023

I think there is a bug with paging up. When I run cargo run more -F100 README.md, the correct lines are shown, however, it is not possible to page up and go to the top of the file. Paging down works fine.

Yes, this is a bug.
In the previous commit, I used to drain the lines that were before the line of start to display.

It should now be resolved.

@cakebaker cakebaker merged commit c524ec4 into uutils:main Jul 1, 2023
@cakebaker
Copy link
Contributor

Thanks!

@Ideflop Ideflop deleted the more_implement_arguments_plain_and_from_line branch July 1, 2023 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants